Add new unused_footnote_definition rustdoc lint
#137858
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
unused_footnote_def
branch
from
c17e182 to
8550b0a
Compare
|
Fixed tidy. |
GuillaumeGomez
force-pushed
the
unused_footnote_def
branch
from
8550b0a to
40b5fa2
Compare
GuillaumeGomez
force-pushed
the
unused_footnote_def
branch
from
40b5fa2 to
27c7ec4
Compare
|
Updated to use new |
|
☔ The latest upstream changes (presumably #140726) made this pull request unmergeable. Please resolve the merge conflicts. |
| let mut footnote_definitions = FxHashMap::default(); | ||
|
|
||
| let options = Options::ENABLE_FOOTNOTES; | ||
| let mut parser = Parser::new_ext(dox, options).into_offset_iter().peekable(); |
There was a problem hiding this comment.
Should we be making sure the lint is enabled before invoking the parser? I know the other lints don't do this, but maybe they should?
| let (ref_span, precise) = | ||
| source_span_for_markdown_range(tcx, dox, &span, &item.attrs.doc_strings) | ||
| .map(|span| (span, true)) | ||
| .unwrap_or_else(|| (item.attr_span(tcx), false)); |
There was a problem hiding this comment.
Couldn't this just be a let Some(ref_span) = ... else { continue }; ?
It doesn't seem like we use ref_span if precise is false.
| Event::Text(text) | ||
| if &*text == "[" | ||
| && let Some((Event::Text(text), _)) = parser.peek() | ||
| && text.trim_start().starts_with('^') | ||
| && parser.next().is_some() | ||
| && let Some((Event::Text(text), end_span)) = parser.peek() | ||
| && &**text == "]" => | ||
| { | ||
| missing_footnote_references.insert(Range { start: span.start, end: end_span.end }); | ||
| } |
There was a problem hiding this comment.
It is quite odd that pulldown_cmark isn't emmitting some form of FootnoteReference here despite the docs saying they might not map to an actual definition.
In any case, I don't think this implementation is correct, since Text events are emitted for the bodies of all blocks. Crucially, this includes code blocks, which certainly should not be parsed for footnotes. An integration test should be added to show that we are not wrongfully parsing footnotes within code blocks.
One way to handle this is to track the type of the last Start event and make sure it isn't CodeBlock. luckily other blocks can't appear within code blocks so we don't have to track the full stack of tags. We might want to clear that variable whenever we reach an End event, but I'm not sure if the text after a code block will always get its own separate Paragraph event or not. An integration test with an unused footnote directly after a code block should clear this up.
| @@ -0,0 +1,9 @@ | |||
| // This test ensures that the rustdoc `unused_footnote` is working as expected. | |||
There was a problem hiding this comment.
| // This test ensures that the rustdoc `unused_footnote` is working as expected. | |
| // This test ensures that the `rustdoc::unused_footnote` lint is working as expected. |
| //! | ||
| //! [^1]: footnote defined | ||
| //! [^2]: footnote defined | ||
| //~^^ unused_footnote_definition |
There was a problem hiding this comment.
| //~^^ unused_footnote_definition | |
| //~^^ ERROR: unused footnote definition |
Nit: for consistency with other tests, look for the actual error and not the note telling you why it is enabled
| "footnote reference with no associated definition" | ||
| } | ||
|
|
||
| declare_rustdoc_lint! { | ||
| /// This lint checks if all footnote definitions are used. | ||
| UNUSED_FOOTNOTE_DEFINITION, | ||
| Warn, | ||
| "unused footnote definition" |
There was a problem hiding this comment.
| "footnote reference with no associated definition" | |
| } | |
| declare_rustdoc_lint! { | |
| /// This lint checks if all footnote definitions are used. | |
| UNUSED_FOOTNOTE_DEFINITION, | |
| Warn, | |
| "unused footnote definition" | |
| "detects footnote references with no associated definition" | |
| } | |
| declare_rustdoc_lint! { | |
| /// This lint checks if all footnote definitions are used. | |
| UNUSED_FOOTNOTE_DEFINITION, | |
| Warn, | |
| "detects unused footnote definitions" |
for consistency with other lint descriptions
Follow-up of #137803 (where the two first commits come from).
It adds a new lint which checks for unused footnote definitions.
r? @notriddle